feat: add optional logger wherever possible #3622
Conversation
This commit introduces an optional logger parameter to various structs. This enhancement allows users to provide custom logging implementations.
An issue will remain for direct calls to internal.Logger.Printf, as the call depth cannot be adjusted there without changing the function signature.
ccoVeille
force-pushed
the
optional-logger
branch
from
03ddd12 to
fa95d65
Compare
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ The following Jit checks failed to run:
- secret-detection-trufflehog
- static-code-analysis-semgrep-pro
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
More info in the Jit platform.
ccoVeille
force-pushed
the
optional-logger
branch
2 times, most recently
from
4e23d33 to
93fa6c4
Compare
ccoVeille
force-pushed
the
optional-logger
branch
from
93fa6c4 to
c963331
Compare
|
I apparently failed to keep your GPG signature @ndyakov, it sounds logic as I used history rewritting to fix an issue in one of the commit I did earlier. Feel free to pull, sign your commits, and push again. My fork is now all set for maintainers edits |
| // Logger: logging.NewLoggerWrapper(logger), | ||
| // }) | ||
| // | ||
| // This will NOT handle all logging at the moment, singe there is still a global logger in use. |
There was a problem hiding this comment.
typo spotted
| // This will NOT handle all logging at the moment, singe there is still a global logger in use. | |
| // This will NOT handle all logging at the moment, since there is still a global logger in use. |
| Infof(ctx context.Context, format string, v ...any) | ||
| Debugf(ctx context.Context, format string, v ...any) | ||
| Enabled(ctx context.Context, level LogLevelT) bool | ||
| } |
There was a problem hiding this comment.
There is something I don't get here with this interface
Either LegacyLogger and LoggerWrapper also support
Error(ctx context.Context, msg string, v ...any)
Warn(ctx context.Context, msg string, v ...any)
Info(ctx context.Context, msg string, v ...any)
Debug(ctx context.Context, msg string, v ...any)Why do you restrict the interface to simili printf logger ?
There was a problem hiding this comment.
those were the only one used, i don't mind adding the non format methods. feel free to add them.
There was a problem hiding this comment.
In fact, I feel like we should consider inverting the logic here.
The custom logger available in the various config should only mention
Error(ctx context.Context, msg string, v ...any)
Warn(ctx context.Context, msg string, v ...any)
Info(ctx context.Context, msg string, v ...any)
Debug(ctx context.Context, msg string, v ...any)
Enabled(ctx context.Context, level LogLevelT) boolSo no Whateverf involved, then the code use the wrapper to transform things.
I feel like asking people to implement Errorf, Infof and co is an error.
There was a problem hiding this comment.
The point of those logger methods is to print a formatted log message, so *f makes sense to me and it is consistent. Users don't have to implement those methods, they do however have to wrap their logger with the provided wrapper if their logger follows the *Context interface like slog.
There was a problem hiding this comment.
I wasn't suggesting to rename things such as:
Infof -> Info
My problem was the fact people will have to add a struct that would satisfy the legacy Printf methods
While we could add this in a way the logger only have to satistify the method that would be used by a structured logger
So the InfoContext one and so on.
The code could adapt itself to use the LoggerWrapper if needed.
Said otherwise people could provide directly a slog.Handler with the v9 as the logger of each config.
The logger wrapper is something we need right now because the code needs the Infof
But I would prefer the magic to be hidden from user. People seeing the interface would think "oh I need to implement Errorf, Warnf and co it's boring and not convenient"
I will add a commit to explain what I have in mind. But later, either this weekend or next week. I will also add tests for all that.
ndyakov
left a comment
ndyakov
left a comment
There was a problem hiding this comment.
Overall looks good to me, can we just make sure this one is addressed (preferably add a simple test):
#3618 (comment)
Here the difference I applied to an old commit. func (cl *LoggerWrapper) Errorf(ctx context.Context, format string, v ...any) {
if cl == nil || cl.logger == nil {
legacyLoggerWithLevel.Errorf(ctx, format, v...)
return
}
- cl.logger.ErrorContext(ctx, format, v...)
+ cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))
}I can add a test, but adding a test about a logger is never fun. Maybe it can be done in a separate PR. I'm suggesting that, but if you are OK to wait for me to add the tests a few more days, maybe a week. I can do it. |
|
@ccoVeille No problem, I will prepare a beta without the logger, you can write tests and resolve conflicts in the next week and then we will include it. |
2 participants
This commit introduces an optional logger parameter to various structs.
This enhancement allows users to provide custom logging implementations.
Fixes #3558
Note
This is a follow-up of #3560 that was merged in a branch (not
master) and #3618 that was updated by @ndyakov to add changes on it.The MR is now ready to be merged.